Conversation
Reviewer's GuideEnhances game log text selection and copying by tracking character-level offsets, recalculating selection ranges on mouseup and select-all, and continuously restoring DOM selection highlights, while removing the previous index-only selection logic. Sequence diagram for enhanced game log text selection and copysequenceDiagram
actor User
participant Browser
participant GameLogPage
participant SelectionState
User->>Browser: mousedown on log line
Browser->>GameLogPage: handleLogMouseDown(index, event)
GameLogPage->>SelectionState: reset range and selecting
GameLogPage->>Browser: window.getSelection().removeAllRanges()
GameLogPage->>Browser: document.caretRangeFromPoint(event.clientX, event.clientY)
Browser-->>GameLogPage: caretRange(startNode, startOffset)
GameLogPage->>SelectionState: store start index, startOffset, end index, endOffset
User->>Browser: mouseup after dragging selection
Browser->>GameLogPage: handleMouseUp()
GameLogPage->>SelectionState: selecting = false
GameLogPage->>Browser: window.getSelection()
Browser-->>GameLogPage: Selection with focusNode, focusOffset
GameLogPage->>GameLogPage: findLogIndex(focusNode)
GameLogPage->>SelectionState: update end index and endOffset
User->>Browser: press Ctrl+A
Browser->>GameLogPage: handleKeyDown(event)
GameLogPage->>SelectionState: set range from first to last log
GameLogPage->>SelectionState: startOffset = 0, endOffset = lastLog.length
User->>Browser: press Ctrl+C
Browser->>GameLogPage: handleCopy(event)
GameLogPage->>SelectionState: read range(start, startOffset, end, endOffset)
GameLogPage->>GameLogPage: normalize start and end
GameLogPage->>GameLogPage: slice filteredLogs[start..end]
GameLogPage->>GameLogPage: trim first and last line by offsets
GameLogPage->>Browser: event.clipboardData.setData("text/plain", text)
GameLogPage->>Browser: event.preventDefault()
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
requestAnimationFrameloop in the selection-restorationuseEffectruns continuously even when there is no selection, which could be expensive; consider only scheduling it while a range exists or whenselectingis true, and stopping when there is nothing to restore. - Using
document.caretRangeFromPointinhandleLogMouseDownmay cause compatibility issues because it is deprecated and not supported in all browsers; it would be safer to feature-detect and fall back to the standarddocument.caretPositionFromPoint/getSelectionAPIs where available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `requestAnimationFrame` loop in the selection-restoration `useEffect` runs continuously even when there is no selection, which could be expensive; consider only scheduling it while a range exists or when `selecting` is true, and stopping when there is nothing to restore.
- Using `document.caretRangeFromPoint` in `handleLogMouseDown` may cause compatibility issues because it is deprecated and not supported in all browsers; it would be safer to feature-detect and fall back to the standard `document.caretPositionFromPoint`/`getSelection` APIs where available.
## Individual Comments
### Comment 1
<location path="src/pages/standalone/game-log.tsx" line_range="342-348" />
<code_context>
+ useEffect(() => {
+ let rafId: number;
+
+ const loop = () => {
+ rafId = requestAnimationFrame(() => {
+ const { range, selecting } = logSelectionStateRef.current;
+ const sel = window.getSelection();
+
+ if (!range || !sel || selecting) {
+ loop();
+ return;
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid an always-on requestAnimationFrame loop when no selection exists
This effect creates a continuous `requestAnimationFrame` loop even when `range` is null, so the selection restoration logic runs every frame for the lifetime of the page. To avoid unnecessary work, only schedule new frames while there is an active selection (or `selecting` is true), and stop re-calling `loop()` once `range` is null and `selecting` is false.
Suggested implementation:
```typescript
useEffect(() => {
let rafId: number;
const loop = () => {
rafId = requestAnimationFrame(() => {
const { range, selecting } = logSelectionStateRef.current;
const sel = window.getSelection();
const hasSelection = !!sel && sel.rangeCount > 0;
// If there's no DOM selection and we're not in an active selection
// gesture, stop scheduling new frames.
if (!hasSelection && !selecting) {
return;
}
// While there is an active selection gesture or we don't yet have a
// restorable range/selection, keep polling.
if (!range || !sel || selecting) {
loop();
return;
}
// Restore the selection range.
sel.removeAllRanges();
sel.addRange(range);
// Continue polling while a selection still exists.
loop();
});
};
loop();
return () => {
if (rafId !== undefined) {
cancelAnimationFrame(rafId);
}
};
}, []);
```
If `logSelectionStateRef` or the selection-restoration behavior depends on additional state (e.g. props or other refs), you may need to:
1. Add those dependencies to the `useEffect` dependency array instead of `[]`.
2. Ensure that `logSelectionStateRef.current.selecting` is set to `false` when the user completes or cancels a selection; otherwise the loop will continue polling as if a selection gesture were still active.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const loop = () => { | ||
| rafId = requestAnimationFrame(() => { | ||
| const { range, selecting } = logSelectionStateRef.current; | ||
| const sel = window.getSelection(); | ||
|
|
||
| if (!range || !sel || selecting) { | ||
| loop(); |
There was a problem hiding this comment.
suggestion (performance): Avoid an always-on requestAnimationFrame loop when no selection exists
This effect creates a continuous requestAnimationFrame loop even when range is null, so the selection restoration logic runs every frame for the lifetime of the page. To avoid unnecessary work, only schedule new frames while there is an active selection (or selecting is true), and stop re-calling loop() once range is null and selecting is false.
Suggested implementation:
useEffect(() => {
let rafId: number;
const loop = () => {
rafId = requestAnimationFrame(() => {
const { range, selecting } = logSelectionStateRef.current;
const sel = window.getSelection();
const hasSelection = !!sel && sel.rangeCount > 0;
// If there's no DOM selection and we're not in an active selection
// gesture, stop scheduling new frames.
if (!hasSelection && !selecting) {
return;
}
// While there is an active selection gesture or we don't yet have a
// restorable range/selection, keep polling.
if (!range || !sel || selecting) {
loop();
return;
}
// Restore the selection range.
sel.removeAllRanges();
sel.addRange(range);
// Continue polling while a selection still exists.
loop();
});
};
loop();
return () => {
if (rafId !== undefined) {
cancelAnimationFrame(rafId);
}
};
}, []);If logSelectionStateRef or the selection-restoration behavior depends on additional state (e.g. props or other refs), you may need to:
- Add those dependencies to the
useEffectdependency array instead of[]. - Ensure that
logSelectionStateRef.current.selectingis set tofalsewhen the user completes or cancels a selection; otherwise the loop will continue polling as if a selection gesture were still active.
Checklist
This PR is a ..
Related Issues
Description
Additional Context
Summary by Sourcery
Fix game log selection and copying behavior and restore full-select support in the standalone log viewer.
Bug Fixes:
Enhancements: